Conversation
provokateurin
requested review from
Altahrim,
come-nc,
leftybournes and
salmart-dev
and removed request for
a team
provokateurin
force-pushed
the
fix/federated-reshare
branch
from
dd38de7 to
d999e97
Compare
| } catch (\Exception $e) { | ||
| // fall back to old re-share behavior if the remote server | ||
| // doesn't support flat re-shares (was introduced with Nextcloud 9.1) | ||
| $this->removeShareFromTable($share); |
There was a problem hiding this comment.
fix: I think that removing this will lead to a stale entry in oc_share if the remote call fails for any reason with an Exception
There was a problem hiding this comment.
IMO this was always wrong. Basically when resharing fails, the original share was also deleted. This was a separate bug besides the fact that resharing didn't work in the first place.
There was a problem hiding this comment.
The original share was deleted because the code causing the exception was setId, but the row in oc_share for Server B to Server C got created in the line above. If the request to the original server fails, we should remove that row, otherwise Server B displays that the file is shared with Server C, but that might not be true.
There was a problem hiding this comment.
There could still be a case where the remote server managed to store the share information but failed afterwards, so removing the share here could lead to an inconsistency in that case, which in connection with my other comment could lead to still having a broken situation 🤔
come-nc
left a comment
come-nc
left a comment
There was a problem hiding this comment.
I agree with @salmart-dev I think. For the rest, approved.
Signed-off-by: provokateurin <kate@provokateurin.de>
…r might on another server Signed-off-by: provokateurin <kate@provokateurin.de>
provokateurin
force-pushed
the
fix/federated-reshare
branch
from
d999e97 to
c8fd59c
Compare
| } catch (\Exception $e) { | ||
| // fall back to old re-share behavior if the remote server | ||
| // doesn't support flat re-shares (was introduced with Nextcloud 9.1) | ||
| $this->removeShareFromTable($share); |
There was a problem hiding this comment.
The original share was deleted because the code causing the exception was setId, but the row in oc_share for Server B to Server C got created in the line above. If the request to the original server fails, we should remove that row, otherwise Server B displays that the file is shared with Server C, but that might not be true.
| } catch (\Exception $e) { | ||
| // fall back to old re-share behavior if the remote server | ||
| // doesn't support flat re-shares (was introduced with Nextcloud 9.1) | ||
| $this->removeShareFromTable($share); |
There was a problem hiding this comment.
There could still be a case where the remote server managed to store the share information but failed afterwards, so removing the share here could lead to an inconsistency in that case, which in connection with my other comment could lead to still having a broken situation 🤔
| $this->storeRemoteId($shareId, $remoteId); | ||
| } else { | ||
| $this->removeShareFromTable($share); | ||
| $this->removeShareFromTable($shareId); |
There was a problem hiding this comment.
comment: noticing that there is no handling for the case where the oc_share entry was missing but the remote had a matching share. This could happen if the current server's DB would be restored to a state without that specific row. I don't see any code that would handle a "share already exists on the remote, track it on this server" but I may have missed it.
3 participants
The following sharing chain didn't work before:
Without these fixes step 4 fails:
server/apps/federatedfilesharing/lib/Controller/MountPublicLinkController.php
Line 124 in dd38de7
$share->setId($shareId);to be called on a share that already has an id set, which fails because it's not allowed. The share from user A to user B is also removed due to bad error handling after the exception.I'm afraid the PublicOwnerWrapper is broken as well, because the code doesn't consider that the owner might be a user from a different instance, but I'm not sure if that can be fixed at all without huge changes. Maybe this scenario could be detected and the initiator could be used as a replacement, but that could also have other unintended side effects.